Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

crypto_algebra.move #6550

Merged
merged 27 commits into from
Apr 7, 2023
Merged

crypto_algebra.move #6550

merged 27 commits into from
Apr 7, 2023

Conversation

zjma
Copy link
Contributor

@zjma zjma commented Feb 10, 2023 •
edited by alinush
Loading

Description

This PR implements a Move module for generic field/group operations. The module is described in this AIP.

How to review this PR

First, I'd recommend looking at the following .move files, in this order:

  1. crypto_algebra.move -- declares functions for most field/group operations
  2. bls12381_algebra.move -- declares struct "marker" types for the BLS12-381 scalar field and groups
  3. groth16.move -- shows how to use the algebra.move module to build a generic Groth16 ZKP verifier that works over any curve; in move-examples/

Second, take a look over some infrastructure changes:

  1. gas_meter.rs -- gas version bump
  2. aptos_framework.rs -- new gas costs for BLS12-381 arithmetic operations
  3. Cargo.toml's -- new dependencies
  4. aptos-move/aptos-vm/src/natives.rs -- now passes some move_stdlib gas parameters down

Third, take a look over the Rust native implementation in aptos-move/framework/src/natives/cryptography/algebra:

  1. mod.rs -- creates all the natives
  2. All the remaining Rust files here implement the natives & add support for BLS12-381 operations

TODOs

  • Hash-to-group implementation.
  • Serialization format documentation currently out-of-date (due to ark library update from 0.3.0 to 0.4.0.
  • Gas parameter specifications.
  • groth16.move
  • Check gas costs are more expensive than Ristretto255 gas costs
  • Check gas costs are consistent with one another
  • Ensure gas costs were derived from single-threaded benchmarks
  • Remove dependency on MUL from gas-scripts & re-compute everything.

Follow-up post-merge

  • High deserialization costs for BLS12-381 in arkworks
  • High hashing costs for BLS12-381 in arkworks
  • Use #[inline] functions for algebra_bls12381.move if appropriate.

movekevin
movekevin reviewed Feb 10, 2023
View reviewed changes
Copy link
Contributor

@movekevin movekevin left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive by comment: Is it possible to separate GenericAlgebraicStructuresBasicOperations and Bls12381Structures into 2 separate PRs? Also please add context to the PR's description

@zjma zjma marked this pull request as draft February 10, 2023 19:26
@zjma
Copy link
Contributor Author

zjma commented Feb 10, 2023

@movekevin sorry, this was supposed to be a draft... converted.

@zjma
Copy link
Contributor Author

zjma commented Feb 10, 2023

Is it possible to separate GenericAlgebraicStructuresBasicOperations and Bls12381Structures into 2 separate PRs?

@movekevin that's possible, but the actual functions work only when both features are on.

@zjma zjma changed the title Algebra.move algebra.move Feb 10, 2023
This was referenced Mar 9, 2023
Merged
Merged
@zjma zjma mentioned this pull request Mar 13, 2023
Merged
@zjma zjma marked this pull request as ready for review March 21, 2023 20:03
alinush
alinush reviewed Apr 4, 2023
View reviewed changes
aptos-move/aptos-gas/src/aptos_framework.rs
@@ -8,6 +8,71 @@ crate::natives::define_gas_parameters_for_natives!(GasParameters, "aptos_framewo
[.account.create_address.base, "account.create_address.base", 300 * MUL],
[.account.create_signer.base, "account.create_signer.base", 300 * MUL],

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vgao1996, be sure to take a look here.

alinush
alinush reviewed Apr 4, 2023
View reviewed changes
aptos-move/framework/src/natives/cryptography/algebra/mod.rs
abort_code: MOVE_ABORT_CODE_NOT_IMPLEMENTED,
}),
}
fn abort_invariant_violated() -> PartialVMError {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vgao1996, we often return this when there's an internal error in our natives, due to us not having maintained the right invariants, or due to a library surprising us with an unexpected return value. Be sure to let us know if this doesn't make sense.

wrwg
wrwg approved these changes Apr 5, 2023
View reviewed changes
Copy link
Contributor

@wrwg wrwg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quite some good work. I skimmed over the way how this is added to Move and the libraries, and see no problem. I left a comment on the AIP which should be resolved (the name algebra for the module is too generic). I wonder whether aptos move test --coverage was attempted?

aptos-move/framework/src/natives/cryptography/algebra/arithmetics/add.rs
assert_eq!(1, ty_args.len());
let structure_opt = structure_from_ty_arg!(context, &ty_args[0]);
abort_unless_arithmetics_enabled_for_structure!(context, structure_opt);
match structure_opt {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's sad that this can't be done on the level of Move via a trait type system, which statically resolves to the right functions. Good example why we later may want this feature in Move.

@zjma
Copy link
Contributor Author

zjma commented Apr 5, 2023

Quite some good work. I skimmed over the way how this is added to Move and the libraries, and see no problem. I left a comment on the AIP which should be resolved (the name algebra for the module is too generic). I wonder whether aptos move test --coverage was attempted?

Yes! It said ~95% coverage.

@zjma zjma enabled auto-merge (squash) April 6, 2023 18:46
@alinush alinush changed the title algebra.move crypto_algebra.move Apr 6, 2023
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions
Copy link
Contributor

github-actions bot commented Apr 7, 2023

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 1f66fdbd6aaa9b2763b6611660be0cf36427340a

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 1f66fdbd6aaa9b2763b6611660be0cf36427340a (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 8137 TPS, 4720 ms latency, 7200 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: 1f66fdbd6aaa9b2763b6611660be0cf36427340a
compatibility::simple-validator-upgrade::single-validator-upgrade : 4848 TPS, 8136 ms latency, 10800 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: 1f66fdbd6aaa9b2763b6611660be0cf36427340a
compatibility::simple-validator-upgrade::half-validator-upgrade : 4731 TPS, 8590 ms latency, 10800 ms p99 latency,no expired txns
4. upgrading second batch to new version: 1f66fdbd6aaa9b2763b6611660be0cf36427340a
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6712 TPS, 5733 ms latency, 9900 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 1f66fdbd6aaa9b2763b6611660be0cf36427340a passed
Test Ok

@github-actions
Copy link
Contributor

github-actions bot commented Apr 7, 2023

✅ Forge suite land_blocking success on 1f66fdbd6aaa9b2763b6611660be0cf36427340a

performance benchmark with full nodes : 5594 TPS, 7086 ms latency, 9600 ms p99 latency,no expired txns
Test Ok

@github-actions
Copy link
Contributor

github-actions bot commented Apr 7, 2023

✅ Forge suite framework_upgrade success on aptos-node-v1.3.0_3fc3d42b6cfe27460004f9a0326451bcda840a60 ==> 1f66fdbd6aaa9b2763b6611660be0cf36427340a

Compatibility test results for aptos-node-v1.3.0_3fc3d42b6cfe27460004f9a0326451bcda840a60 ==> 1f66fdbd6aaa9b2763b6611660be0cf36427340a (PR)
Upgrade the nodes to version: 1f66fdbd6aaa9b2763b6611660be0cf36427340a
framework_upgrade::framework-upgrade::full-framework-upgrade : 6421 TPS, 6019 ms latency, 9000 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for aptos-node-v1.3.0_3fc3d42b6cfe27460004f9a0326451bcda840a60 ==> 1f66fdbd6aaa9b2763b6611660be0cf36427340a passed
Test Ok

@zjma zjma merged commit 388e3bc into main Apr 7, 2023
@zjma zjma deleted the algebra.move branch April 7, 2023 17:11
@alinush alinush mentioned this pull request Apr 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@movekevin movekevin movekevin left review comments

@pr-aptos pr-aptos pr-aptos left review comments

@mstraka100 mstraka100 mstraka100 approved these changes

@alinush alinush alinush approved these changes

@wrwg wrwg wrwg approved these changes

@davidiw davidiw Awaiting requested review from davidiw davidiw is a code owner

@zekun000 zekun000 Awaiting requested review from zekun000 zekun000 is a code owner

@clay-aptos clay-aptos Awaiting requested review from clay-aptos

@saharct saharct Awaiting requested review from saharct

@junkil-park junkil-park Awaiting requested review from junkil-park junkil-park is a code owner

@vgao1996 vgao1996 Awaiting requested review from vgao1996 vgao1996 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@zjma @alinush @wrwg @mstraka100 @pr-aptos @movekevin